USHIFT-6912: Add DNS deployment resource configuration support#6621
USHIFT-6912: Add DNS deployment resource configuration support#6621Neilhamza wants to merge 9 commits intoopenshift:mainfrom
Conversation
Allow users to configure CPU and memory resources for the dns container in the dns-default DaemonSet via dns.resources in config.yaml. Defaults to the current hardcoded values (cpu=50m, memory=70Mi requests, no limits) when not configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Neilhamza: This pull request references USHIFT-6912 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd DNS container resource configuration: new config types and defaults; merge and validate user requests/limits; controller exposes template params; DaemonSet templates request values and optional limits; update schema, packaging, docs, and tests. ChangesDNS Resource Configuration
Sequence DiagramsequenceDiagram
participant User
participant ConfigService
participant Controller
participant Kubernetes
participant Pod
User->>ConfigService: write/edit `dns.resources` drop-in
ConfigService->>Controller: incorporateUserSettings (merge requests/limits)
Controller->>Kubernetes: render DaemonSet template with DNS requests/limits params
Kubernetes->>Pod: create/update CoreDNS Pod with resources set
Pod->>Kubernetes: report Pod status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/howto_config.md`:
- Around line 207-209: The "Default Settings" example shows an empty requests
object ("requests: {}") which is misleading because the system applies defaults;
update the example in the Default Settings section so the effective DNS request
defaults are explicit (e.g., set requests to include cpu: 50m and memory: 70Mi)
and add a short note next to the example clarifying that these values reflect
the system's applied defaults; locate the block containing "resources:" /
"limits:" / "requests:" in the docs and replace the empty requests with the
explicit default key/value pairs and the clarifying note.
In `@pkg/config/dns.go`:
- Around line 131-153: The validateResources method on DNS should reject any
resource keys other than the allowed set ("cpu" and "memory"); update
DNS.validateResources to iterate Requests and Limits and return an error if a
key is not "cpu" or "memory" (before parsing), ensuring both
t.Resources.Requests and t.Resources.Limits are checked, and keep the existing
quantity parsing and comparison logic (use the same error formatting style) so
downstream templating in components/controllers.go and
assets/components/openshift-dns/dns/daemonset.yaml only sees cpu/memory keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 67ea07fa-e704-476d-83a9-5d9cae789b63
📒 Files selected for processing (8)
assets/components/openshift-dns/dns/daemonset.yamlcmd/generate-config/config/config-openapi-spec.jsondocs/user/howto_config.mdpackaging/microshift/config.yamlpkg/components/controllers.gopkg/config/config.gopkg/config/config_test.gopkg/config/dns.go
Reject unsupported resource keys during config validation to prevent typos and ensure only cpu/memory are accepted, matching what the DaemonSet template renders. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
|
hi @Neilhamza thank you very much for creating the PR early in the dev cycle. |
|
Thanks for the feedback @agullon !! i was thinking about a seperate PR so i wont bloat this one, but if adding the tests here is the accepted approach i can do it, what do you think? :) |
|
/test e2e-aws-tests |
I don't have a strong opinion about when/where add the RF, it's up to you. As I said I think it's better to add them along with product change. This PR is not that big, no problem. |
RobotFramework tests covering: default resources, custom requests and limits, requests only, partial requests, invalid quantity rejection, and DNS resolution after resource change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai perform full review |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
|
@agullon added RF testscoverage - : default resources, custom requests+limits, requests only, partial requests, invalid quantity rejection, and DNS resolution after change |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/suites/configuration/dns-resource-configuration.robot (2)
141-141: ⚡ Quick winMake restart-failure assertion less brittle.
Line [141] hardcodes
0 != 1, which tightly couples the test to exact assertion text/order. A wildcard pattern is more resilient to harmless keyword-message changes.Proposed change
- Run Keyword And Expect Error 0 != 1 Restart MicroShift + Run Keyword And Expect Error *!=* Restart MicroShift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/configuration/dns-resource-configuration.robot` at line 141, Replace the brittle exact-error match in the test's Run Keyword And Expect Error call: instead of hardcoding the literal '0 != 1' use a wildcard pattern (e.g. '*') so the assertion matches any error message from the Restart MicroShift keyword; update the Run Keyword And Expect Error invocation that currently contains '0 != 1' and 'Restart MicroShift' to use a wildcard error pattern.
42-115: ⚡ Quick winAdd a negative case for
limit < request.The suite validates bad quantity parsing, but it does not yet exercise the
limits >= requestsvalidation path inpkg/config/dns.go. Adding this guards an important contract from regressions.Suggested test addition
+${DNS_INVALID_LIMIT_LT_REQUEST} SEPARATOR=\n +... --- +... dns: +... \ \ resources: +... \ \ \ requests: +... \ \ \ \ cpu: "200m" +... \ \ \ limits: +... \ \ \ \ cpu: "100m" + +Limit Lower Than Request Prevents Start + [Documentation] Verify MicroShift fails when dns limit is lower than request + [Setup] Apply Invalid DNS Resource Config ${DNS_INVALID_LIMIT_LT_REQUEST} + Pattern Should Appear In Log Output ${CURSOR} must be greater than or equal to request + [Teardown] Restore Valid DNS Config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/configuration/dns-resource-configuration.robot` around lines 42 - 115, Add a new negative Robot test case that verifies the config validation rejects entries where a resource limit is less than the corresponding request (exercising the limits >= requests path in pkg/config/dns.go); create a test named like "Invalid Limits Less Than Requests" that uses the existing helper keywords (Apply DNS Resource Config or a new Apply Invalid DNS Resource Config with a YAML payload where cpu limit < cpu request and/or memory limit < memory request), assert the controller/log contains the appropriate error (e.g., "invalid dns resource request" or similar via Pattern Should Appear In Log Output ${CURSOR}) and clean up with the existing teardown keyword (Restore Valid DNS Config or Remove DNS Resource Config) so the suite remains isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/suites/configuration/dns-resource-configuration.robot`:
- Line 141: Replace the brittle exact-error match in the test's Run Keyword And
Expect Error call: instead of hardcoding the literal '0 != 1' use a wildcard
pattern (e.g. '*') so the assertion matches any error message from the Restart
MicroShift keyword; update the Run Keyword And Expect Error invocation that
currently contains '0 != 1' and 'Restart MicroShift' to use a wildcard error
pattern.
- Around line 42-115: Add a new negative Robot test case that verifies the
config validation rejects entries where a resource limit is less than the
corresponding request (exercising the limits >= requests path in
pkg/config/dns.go); create a test named like "Invalid Limits Less Than Requests"
that uses the existing helper keywords (Apply DNS Resource Config or a new Apply
Invalid DNS Resource Config with a YAML payload where cpu limit < cpu request
and/or memory limit < memory request), assert the controller/log contains the
appropriate error (e.g., "invalid dns resource request" or similar via Pattern
Should Appear In Log Output ${CURSOR}) and clean up with the existing teardown
keyword (Restore Valid DNS Config or Remove DNS Resource Config) so the suite
remains isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e441d468-d579-4a05-8012-d779622e04f5
📒 Files selected for processing (1)
test/suites/configuration/dns-resource-configuration.robot
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| {{- if .DNSHasLimits }} | ||
| limits: | ||
| {{- if .DNSCPULimit }} | ||
| cpu: {{ .DNSCPULimit }} | ||
| {{- end }} | ||
| {{- if .DNSMemoryLimit }} | ||
| memory: {{ .DNSMemoryLimit }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Why not just give customers whole resources: {}? If they define anything, we take it whatever they provide. Would simplify templating greatly
There was a problem hiding this comment.
I agree . since resource is something generic we might be able to reuse it for different components besides dns. (ie ingress)
There was a problem hiding this comment.
Nice! simplified the template to pass resource map directly with range loops of 2 params instead of 7 and is reusable for other components
|
/assign |
|
/assign @eslutsky |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Pass resource maps directly instead of individual values, use range loops in the template. Reduces params from 7 to 2 and makes the pattern reusable for other components. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Neilhamza The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reject resource requests below the defaults (cpu: 50m, memory: 70Mi) to prevent unstable clusters from insufficient DNS resources. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify that configuring only limits preserves default requests, covering the corner case raised in enhancement review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| *** Test Cases *** | ||
| Default DNS Resources | ||
| [Documentation] Verify default DNS resources when no custom config is applied |
There was a problem hiding this comment.
| [Documentation] Verify default DNS resources when no custom config is applied | |
| [Documentation] Verify default DNS resources when no custom config is applied | |
| [Setup] Remove DNS Resource Config |
Tests must be indempotent, meaning regardless the previous status of the system they must always pass. For that reason the test must enforce default DNS config before checking it.
There was a problem hiding this comment.
Thanks for the detailed review @agullon , i committed this suggested change
|
|
||
| Partial Requests Preserves Defaults | ||
| [Documentation] Configure only CPU request and verify memory default is preserved | ||
| [Setup] Apply DNS Resource Config ${DNS_PARTIAL_REQUESTS} |
There was a problem hiding this comment.
the setup of this test only set requests.cpuin the config.
What would happen if requests.memory was already set to a value different than 70Mi (default) before this test starts? I think it would failed because the different value will still be present when the check is done.
To avoid this test must remove all configurations and then apply any config before doing any check.
Something not great about this suggestion is that MicroShift will restart many times (I think 3) on every test, which will lead into long test executions. Checks take less than a second. Successful microshift restart at least 1 min. We must have this in mind. An alternative may be running all the tests in a predefined order to reduce the number of times microshift restarts.
There was a problem hiding this comment.
fixed - now we remove any existing drop-in before applying the new one
| *** Test Cases *** | ||
| Default DNS Resources | ||
| [Documentation] Verify default DNS resources when no custom config is applied | ||
| ${cpu}= Oc Get JsonPath daemonset openshift-dns dns-default |
There was a problem hiding this comment.
Oc Get JsonPath daemonset openshift-dns dns-default is duplicated 15 times. Can you move it into a Keyword
There was a problem hiding this comment.
done created Get DNS Resource Value keyword that wraps it
| ${cpu}= Oc Get JsonPath daemonset openshift-dns dns-default | ||
| ... .spec.template.spec.containers[0].resources.requests.cpu | ||
| ${memory}= Oc Get JsonPath daemonset openshift-dns dns-default | ||
| ... .spec.template.spec.containers[0].resources.requests.memory |
There was a problem hiding this comment.
You may also want to extract .spec.template.spec.containers[0].resources into a var ${DNS_RESOURCE_PATH} .spec.template.spec.containers[0].resources and then use it ${DNS_RESOURCE_PATH}.requests.cpu
|
/jira refresh |
|
@Neilhamza: This pull request references USHIFT-6912 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
- Make tests idempotent by cleaning config in setup before checks - Extract duplicate jsonpath into Get DNS Resource Value keyword - Add DNS_RESOURCE_PATH variable for the base container path - Clean existing drop-in before applying new config in Apply keyword Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| Remove Drop In MicroShift Config ${DNS_DROPIN} | ||
| Drop In MicroShift Config ${config} ${DNS_DROPIN} |
There was a problem hiding this comment.
Inside Drop In MicroShift Config MicroShift config is being removed.
By only adding Remove Drop In MicroShift Config before Drop In MicroShift Config you are not changing any MicroShift config, only some files. This is because MicroShift config is only update it when microshift restarts. MicroShift must always restart after any config change.
| Remove Drop In MicroShift Config ${DNS_DROPIN} | |
| Drop In MicroShift Config ${config} ${DNS_DROPIN} | |
| Remove DNS Resource Config | |
| Drop In MicroShift Config ${config} ${DNS_DROPIN} |
| Apply Invalid DNS Resource Config | ||
| [Documentation] Apply an invalid DNS resource config that should prevent MicroShift from starting | ||
| [Arguments] ${config} | ||
| Remove Drop In MicroShift Config ${DNS_DROPIN} |
There was a problem hiding this comment.
| Remove Drop In MicroShift Config ${DNS_DROPIN} | |
| Remove DNS Resource Config |
| Remove DNS Resource Config | ||
| [Documentation] Remove the DNS resource drop-in config and restart MicroShift | ||
| Remove Drop In MicroShift Config ${DNS_DROPIN} | ||
| Restart MicroShift | ||
|
|
||
| Restore Valid DNS Config | ||
| [Documentation] Remove invalid config and restore MicroShift to a working state | ||
| Remove Drop In MicroShift Config ${DNS_DROPIN} | ||
| Restart MicroShift |
There was a problem hiding this comment.
These 2 are the same, let's use only one.
|
@Neilhamza: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Allow users to configure CPU and memory resources for the dns container in the dns-default DaemonSet via dns.resources in config.yaml. Defaults to the current hardcoded values (cpu=50m, memory=70Mi requests, no limits) when not configured.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests